Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rn): get login working e2e #1132

Merged
merged 1 commit into from
Nov 13, 2024
Merged

fix(rn): get login working e2e #1132

merged 1 commit into from
Nov 13, 2024

Conversation

moldy530
Copy link
Collaborator

@moldy530 moldy530 commented Nov 6, 2024

Pull Request Checklist


PR-Codex overview

This PR focuses on enhancing the NativeTEKStamper integration within the React Native application, improving module exports, and updating dependencies.

Detailed summary

  • Changed export of NativeTEKStamper from default to named export.
  • Updated Android module configuration for isTurboModule.
  • Added dotenv configuration in build.gradle.
  • Updated package.json with new dependencies.
  • Enhanced SignerClient class with new methods for authentication.
  • Improved NativeTEKStamperModule with HPKE decryption support.
  • Updated various dependencies in yarn.lock for better compatibility.

The following files were skipped due to too many changes: account-kit/rn-signer/example/yarn.lock

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented Nov 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
aa-sdk-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 13, 2024 0:21am
aa-sdk-ui-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 13, 2024 0:21am

Copy link
Collaborator Author

moldy530 commented Nov 6, 2024

Comment on lines +100 to +103
if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME).javaClass != BouncyCastleProvider::class.java) {
Security.removeProvider(BouncyCastleProvider.PROVIDER_NAME)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was due to the fact that android adds its own bouncy castle implementation that doesn't include EC

)
.apply()

return promise.resolve(true)
} catch (e: Exception) {
Log.e("error", "an error happened", e)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, remove this


export default function App() {
const [publicKey, setPublicKey] = useState<string | null>(null);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [eslint] <@typescript-eslint/no-unused-vars> reported by reviewdog 🐶
'publicKey' is assigned a value but never used.

}

return HpkePrivateKey.create(
com.google.crypto.tink.hybrid.HpkePublicKey.create(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use the proto version of the public key elsewhere hence the need for this specific one. I'm going to look into refactoring things so we can use the same interfaces throughout

return DatatypeConverter.printHexBinary(outputStream.toByteArray()).uppercase()
}

private fun convertToUncompressedPublicKeyBytes(ephemeralPublicKey: ECPublicKey): ByteArray {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna follow up and actually make this a helper that takes in: key: bytes, inputFormat: PointFormatType, outputFormat: PointFormatType to make things a bit cleaner

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a super barebones signer implementation that needs to move out of the example and in to the RN src, have this here to test

Copy link
Collaborator Author

moldy530 commented Nov 6, 2024

Was able to:

  • generate a link with a bundler
  • copy the bundle in
  • complete email auth!

(bundle omitted from image, the user: ... is what we returned from our servers on successful whoami request post login)
image.png

Copy link
Contributor

@dphilipson dphilipson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy cow this is intense. Great work!


useEffect(() => {
const init = async () => {
console.log("init again");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to take this out this log statement and the one below before merging (unless they're intentional).

console.log("publicKey", publicKey);
setPublicKey(publicKey);
})
.catch(console.error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The async-await is unnecessary. You can either just put this promise-then-catch at the top level of the useEffect callback or keep the async-await and use a try-catch block.

bundle,
authenticatingType: "email",
connectedEventName: "connectedEmail",
orgId: orgId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Very nit) No need to repeat orgId here.

});
}

createAccount(params: CreateAccountParams): Promise<SignupResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using the override keyword for all the abstract method implementations. It helps with readability and prevents errors if the signature don't match.


export default NativeTEKStamper;
export { NativeTEKStamper };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be

export { NativeTEKStamper } from "./NativeTEKStamper";

?

Base automatically changed from moldy/rn-stamping to moldy/rn-base November 13, 2024 00:27
@moldy530 moldy530 merged commit ee03d62 into moldy/rn-base Nov 13, 2024
3 of 7 checks passed
@moldy530 moldy530 deleted the moldy/rn-login branch November 13, 2024 00:27
@iykazrji
Copy link
Collaborator

Just going through this! This is glorious @moldy530 ! 🔥🔥🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants